-
-
Notifications
You must be signed in to change notification settings - Fork 376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Export exit codes to JSON output #371
Export exit codes to JSON output #371
Conversation
src/hyperfine/benchmark.rs
Outdated
/* From the ExitStatus::code documentation: | ||
"On Unix, this will return None if the process was terminated by a signal." | ||
On the other configurations, ExitStatus::code should never return None. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but I don't really see a guarantee for the latter part ("On the other configurations, ExitStatus::code should never return None.") in the documentation.
I think it might be best to simply return an Option<i32>
from this function and later (in the export) convert exit codes to a string that either contains the exit code or an empty string.
This would also allow us to remove all the unwraps.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JordiChauzi Are you interested in finishing this? Otherwise, that's also fine! We can find someone else then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about the delay. Last month was a little busy and I kind of forgot about this PR.
Anyway, I will try and implement the changes in the next days.
You are right, there is no real guarantee that a configuration may return None
from the ExitStatus::code
method (this is currently not the case but again, this is not a guarantee). Printing an empty string in that case makes sense. Do we also want to store None
instead of unwrapping when the ExitStatus::signal
method returns None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default behavior for serde
and outputting json
is to serialize None
as null
. This will create outputs that look like this:
...
"exit_codes": [
0,
0,
0,
1,
null,
null,
42,
0,
0,
0
]
...
To me, it looks "good enough". If you still want the empty string behavior tell me so. If this is the case, I think I will have to add a custom serialize method pour the exit_codes
field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I like the null
solution better. You are right. Thanks!
Thank you very much for your contribution! This looks great. I added one inline comment. |
src/hyperfine/benchmark.rs
Outdated
"On Unix, this will return None if the process was terminated by a signal." | ||
In that case, ExitStatusExt::signal should never return None. | ||
*/ | ||
status.code().or_else(|| status.signal()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should either return the exit code (if it's there) OR the signal n + 128. This is a standard procedure, e.g. in Bash: https://tldp.org/LDP/abs/html/exitcodes.html
Returning just the signal number (e.g. 9
for KILL
) could be confused with the actual exit code 9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right! I could not easily find a constant in libc
or in the std
library, so I hardcoded 128
with a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Fix #348.
I added the field
exit_codes: Vec<i32>
to theBenchmarkResult
struct.However, on
unix
, the returnedExitStatus
can beNone
, when the process is terminated by a signal. In that case, I used thesignal
method result (fromExitStatusExt
forunix
) as exit code. This part is located in theextract_exit_code
function inbenchmark.rs
. I tried to document theunwrap
uses (I found other instances ofunwrap
in the library so I assumed it was ok to use it here).